-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(page): Add toggle for full width page view #874
Conversation
187fef8
to
c16f775
Compare
5 failed and 4 flaky tests on run #1150 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nice! Notes from the design review:
- Move "Last changed by" item to the top
- Add divider before and after "Full width" and "Show outline"
Jane Doe an hour ago
---
Full width
Show outline
---
Add template for subpages
Show in Files
Delete page
- Bug: if full-width is toggled, the action menu does not close, but 3 dot menu moves --> to be fixed in component
- Temporary solution: Always show the group of 3 buttons (Edit, 3dot menu, sidebar toggle) at the right end of the page
- Group of buttons should have equal top and right margin
- Nice-to-have: buttons are aligned with top right header buttons (avatar, notifications, contact search, universal search)
27f662b
to
c4ebae3
Compare
@nimishavijay I addressed all the topics we discussed in the call, please see the updated screenshots |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super! :)
@KaeTuuN @theron29 since you commented in the issue this fixes, would this "per-page per-device" setting work for you? @mejo- I’m leaning a bit towards that the layout should be a per-device setting across pages, since per the issue it seems it’s mostly people with large screens, and that doesn’t change across pages. |
From my perspective it is more the actual page content that would influence if you want to use the full width or regular width mode, e.g. for pages with long text you might prefer a limited width for readability while for pages with larger tables or other content a full width is more suitable. I think mid term we should also adapt this in text itself, so having it per document could also be stored in the file rather than per user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a quick look at the code. Looks good all in all.
Just one minor thing that needs to be addressed before the merge.
@jancborchardt Yes, this actually looks very good and promising 👍 |
c4ebae3
to
1412a42
Compare
1412a42
to
244fda6
Compare
Failing Cyress tests are related, will look into it next week. |
@jancborchardt Thanks for mentioning me! First: Thanks for developing! (@mejo- )
Greetings, |
I don't quite understand where you would put it. Could you edit one of the screenshots above with an indication where you would expect to find this feature? |
I'm on my Smartphone atm. Greetings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the additional explanation @juliushaertl, then looks good design-wise. :)
And the setting is good in the menu, moving it out would make it too present compared to how important it is.
Fixes: #242 Signed-off-by: Jonas <[email protected]>
Signed-off-by: Jonas <[email protected]>
Signed-off-by: Jonas <[email protected]>
244fda6
to
e7a92fd
Compare
Sure 😊
Well, as @jancborchardt already said, this would be a bit too prominent. We try to not clutter the user interface with too many buttons and switches to not overwhelm users. |
Also add same top and right margin to titlebar. Signed-off-by: Jonas <[email protected]>
We might want to use this within Text later on, so let's already prefix it with `text-` instead of `collectives-`. Signed-off-by: Jonas <[email protected]>
641cfbb
to
7f6a6b7
Compare
The full width mode is a per-page user setting and is persisted in browser storage. It will not persist across devices.
Fixes: #242
Screenshots
🏁 Checklist
npm run lint
/npm run stylelint
/composer run cs:check
)